Fall back to global trace config when a span's own config is null - #12374
Fall back to global trace config when a span's own config is null#12374dougqh wants to merge 1 commit into
Conversation
AgentTracer.traceConfig(AgentSpan) returned null for an ExtractedSpan (a span wrapping only a remote/propagated context, not yet a local span), since ExtractedSpan.traceConfig() intentionally reports null. Several logging instrumentations (logback, log4j1, log4j2, jboss-logmanager, tinylog) call traceConfig(span).isLogsInjectionEnabled() without a further null check, so this surfaced as an NPE swallowed by the tracer's own advice-exception handler and misattributed in error tracking to whatever code happened to be on the calling thread. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
||
| public static TraceConfig traceConfig(final AgentSpan span) { | ||
| return null != span ? span.traceConfig() : traceConfig(); | ||
| final TraceConfig config = span == null ? null : span.traceConfig(); |
There was a problem hiding this comment.
span.traceConfig returns null for ExtractedSpan.
Arguably, we'd be better fixing ExtracedSpan, but I'm curious what others think.
There was a problem hiding this comment.
IMHO fixing ExtractedSpan to return AgentTracer.traceConfig() would be better.
b699de3 to
83e57f8
Compare
dd-octo-sts
Bot
added
the
tag: ai generated
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The fallback keeps a span-specific trace configuration. It uses the global trace configuration only when the span or its configuration is null.
🤖 Datadog Autotest · Commit 83e57f8 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
2 participants
What Does This Do
AgentTracer.traceConfig(AgentSpan)returnednullwhenever the given span itself reported anullTraceConfig(e.g. anExtractedSpan, which wraps only a remote/propagated context and has no local trace config). It now falls back to the global trace config in that case, the same fallback the method already used when the span itself wasnull.Added
AgentTracerTestcovering: anullspan, a span with its own config, and the regression case (an extracted span whose owntraceConfig()isnull).Motivation
Several logging instrumentations call
traceConfig(span).isLogsInjectionEnabled()after only null-checkingspan(not the returnedTraceConfig), e.g.LogbackLoggerInstrumentation$CallAppendersAdvice:When the active span is an
ExtractedSpan,traceConfig(span)returnednull, causing aNullPointerException. It's swallowed by the tracer's own advice-exception handler so it never breaks the instrumented app, but it shows up as a spurious error-tracking issue, misattributed to whatever code happens to be on the calling thread at the time (in the case that prompted this fix, an rxjava2 observer chain, sinceLogger.callAppenderswas invoked synchronously from deep inside it).The same unguarded pattern is duplicated across five logging integrations (logback, log4j1, log4j2, jboss-logmanager, tinylog), so fixing it at the shared source (
AgentTracer.traceConfig) covers all of them at once instead of patching each call site.Additional Notes
Investigated via a Datadog Error Tracking issue whose stack trace pointed at
datadog.trace.instrumentation.rxjava2.TracingSingleObserver.onSuccess— that attribution was a red herring; root-cause writeup posted as a comment on the issue.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueJira ticket: [none]